[libfyaml] new port, [appstream] update to 1.1.2#51490
[libfyaml] new port, [appstream] update to 1.1.2#51490brunvonlope wants to merge 2 commits intomicrosoft:masterfrom
Conversation
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
3 times, most recently
from
a2cc5c0 to
a38f3a7
Compare
|
Drafting due to build failures in the new port. |
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
4 times, most recently
from
70c3ef2 to
778012a
Compare
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
6 times, most recently
from
3e113bd to
91a61d4
Compare
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
3 times, most recently
from
492d07b to
f41ce33
Compare
|
@BillyONeal May be in decent shape for review now :) |
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
6 times, most recently
from
48b015e to
483ad2b
Compare
BillyONeal
left a comment
BillyONeal
left a comment
There was a problem hiding this comment.
I believe GPT 5.4 is correct about this:
The main review findings are bundled third-party code with likely incomplete license metadata and host-dependent optional components that are not explicitly controlled by the port.
Declared Metadata
- License:
MIT—ports\libfyaml\vcpkg.json:6
Build Invocation Summary
- Primary build helper(s):
vcpkg_cmake_configure,vcpkg_cmake_install,vcpkg_fixup_pkgconfig,vcpkg_copy_tools,vcpkg_cmake_config_fixup—ports\libfyaml\portfile.cmake:31-44 - Key options:
-DBUILD_TESTING=OFF—ports\libfyaml\portfile.cmake:31-35
Vendored Dependencies
-
Bundled component:
xxHash- Evidence:
buildtrees\libfyaml\src\...\src\xxhash\xxhash.c; compiled intofyamlviaCMakeLists.txt:869; BSD-2-Clause notice insrc\xxhash\xxhash.h:9-35 - Status: installed as part of the shipped library
- Assessment: bundled third-party code is used in the library, but it is not modeled in
ports\libfyaml\vcpkg.json
- Evidence:
-
Bundled component: Windows
getopt- Evidence:
CMakeLists.txt:1246-1249addssrc\getopt\getopt.con Windows;fy-toolis installed viaCMakeLists.txt:1684-1690and copied byports\libfyaml\portfile.cmake:42; BSD-3-Clause text insrc\getopt\LICENSE.getopt - Status: installed indirectly in the shipped
fy-tool.exe - Assessment: bundled Windows-only dependency, also not modeled in metadata
- Evidence:
-
Bundled component:
BLAKE3- Evidence:
CMakeLists.txt:877-907,897compilessrc\blake3\*intofyamland installsinclude\libfyaml\libfyaml-blake3.h; installed package containsinclude\libfyaml\libfyaml-blake3.h - Status: installed as part of the shipped library and public headers
- Assessment: bundled code is definitely shipped; its third-party notice coverage should be reviewed
- Evidence:
Optional Dependency Risks
-
Dependency / feature:
libyaml- Evidence: upstream auto-detects it with
find_package(yaml QUIET CONFIG)/pkg_check_modules(LIBYAML yaml-0.1)—buildtrees\libfyaml\src\...\CMakeLists.txt:210-227 - Why it is risky:
ports\libfyaml\vcpkg.jsondoes not declareyaml, andports\libfyaml\portfile.cmakedoes not explicitly disable or gate it. If present on the host, it changes the build by enablinglibfyaml-parser—CMakeLists.txt:1300-1335 - Suggested packaging change: explicitly disable this path or add a feature/dependency for it
- Evidence: upstream auto-detects it with
-
Dependency / feature:
Sphinx- Evidence: upstream does
find_program(SPHINX_EXECUTABLE)and conditionally installs generated manpages —CMakeLists.txt:1592-1654; otherwise it installs a canned fallback manpage —CMakeLists.txt:1665-1680 - Why it is risky: installed content varies with host tooling; builders with
sphinx-buildcan install extra manpages that are absent otherwise - Suggested packaging change: patch docs off, or force the canned-manpage path for reproducible packaging
- Evidence: upstream does
License / Installed Content Findings
-
Finding: declared
MITappears incomplete for the shipped contents- Declared license:
MIT - Observed installed content:
packages\libfyaml_x64-windows\bin\fyaml.dll,packages\libfyaml_x64-windows\tools\libfyaml\fy-tool.exe,packages\libfyaml_x64-windows\share\libfyaml\copyright - Evidence: installed copyright is only upstream MIT text; bundled
xxHashcarries BSD-2-Clause text (src\xxhash\xxhash.h:9-35), and bundled Windowsgetoptcarries BSD-3-Clause text (src\getopt\LICENSE.getopt) - Assessment: the port likely needs a broader SPDX expression and/or bundled third-party notices in the installed copyright
- Declared license:
-
Finding: bundled
BLAKE3code is shipped but not obviously covered by installed notices- Declared license:
MIT - Observed installed content:
packages\libfyaml_x64-windows\include\libfyaml\libfyaml-blake3.h - Evidence:
src\blake3\*is compiled intofyamland the public BLAKE3 header is installed —CMakeLists.txt:877-907,897 - Assessment: verify upstream licensing for the
src\blake3subtree and include any required attribution in the package metadata/notices
- Declared license:
Other Port Review Suggestions
- Suggestion: consider feature-gating the CLI tool
- Evidence:
fy-toolis always installed by upstream (CMakeLists.txt:1684-1690) and always copied by the port (ports\libfyaml\portfile.cmake:42) - Rationale: if the intended default is “library-only”, a
toolsfeature would reduce package surface and avoid always shipping the extra executable
- Evidence:
Recommended Follow-ups
- Audit the bundled
xxhash,getopt, andblake3subtrees and updatelicense/ installed notices accordingly. - Make
libyamldetection explicit: either disable it or model it as a feature with a declared dependency. - Remove host-tool variance from docs packaging by explicitly disabling Sphinx-generated docs or forcing the canned-manpage path.
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
from
483ad2b to
8c5df4b
Compare
|
@BillyONeal I appreciate the effort to make a review with the chatbot but would prefer a human, non overly verbose, and actionable review |
Needed for appstream port
brunvonlope
force-pushed
the
bruno/libfyaml-test
branch
from
8c5df4b to
84348da
Compare
|
Re-requested human review |
The vendored dependencies need to be dealt with, the incorrect license claims need to be fixed (some of which may be resolved by devendoring), and the optional dependencies need to be correctly controlled. I already removed incorrect statements the bot made and removed irrelevant parts of the report. Of what remains, the only difference between what I would write and what the bot wrote is (1) I would have probably missed some of this, which would have likely resulted in merging this with those mistakes, which would have likely resulted in needing to immediately deindex this as soon as a build with it and one of the vendored parts was observed together, and (2) even if I had found these I would have been less meticulous about documenting evidence of the vendored dependencies, I would have just said "you need to devendor xxhash, getopt, and blake3 and control at least the libyaml optional dependencies". I agree that "I gave it to the chat bot and posted slop" can be a problem. I'm not treating these things as "god" and frequently resolve incorrect comments on their behalf for contributors, for example #51542 (comment) I list when LLMs have been used in authoring some or all of the report as attribution and disclosure; the point is more that when I post something that does not have an LLM disclosure in it, I did not use an LLM for it. |
|
I have no problem with the tool you use as long it don't vomit a lot of text on a later stage of the reviewing. I will take a look, anyway |
3 participants
Supersedes #49474
./vcpkg x-add-version --alland committing the result.